Skip to content

OID4VC: Nonce validation, .well-known path, readme#3151

Merged
jamshale merged 5 commits into
openwallet-foundation:mainfrom
weiiv:oid4vc
Jun 4, 2026
Merged

OID4VC: Nonce validation, .well-known path, readme#3151
jamshale merged 5 commits into
openwallet-foundation:mainfrom
weiiv:oid4vc

Conversation

@weiiv

@weiiv weiiv commented May 26, 2026

Copy link
Copy Markdown
Contributor
  • conditional nonce in 4 different mode through configure
  • .well-known path to align with v1.0 spec
  • readme for nonce configure

weiiv added 3 commits May 26, 2026 12:38
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
@weiiv

weiiv commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Hi @jamshale, when you have a moment, could you please take a look at this small PR and approve if everything looks good? Thanks so much! -Ivan

@swcurran

Copy link
Copy Markdown
Contributor

Hey @weiiv -- I had Claude review the PR, and the response requires your review and response, I think. A couple of things seem odd. Can you take a look, please? Sorry for the long description...typical AI. :-(

This PR makes three related changes to the OID4VC plugin:

Conditional nonce endpoint — a new OID4VCI_ENABLE_NONCE_ENDPOINT config flag lets operators disable the /nonce endpoint. When disabled, nonce validation falls back to comparing the c_nonce from the access token directly rather than using DB-based single-use redemption.
.well-known path fix — moves the tenant subpath after the .well-known prefix (e.g. /.well-known/openid-credential-issuer/tenant/{id} instead of /tenant/{id}/.well-known/…) to align with RFC 8615 and OID4VCI 1.0.
README documentation — explains the four deployment scenarios and the new config flag.

Issues

  1. SECURITY — init.sql hardcodes credentials (regression)
    oid4vc/auth_server/alembic/sql/init.sql

The previous version used psql variable placeholders (:ADMIN_DB_USER, :ADMIN_DB_PASSWORD, :ADMIN_DB_NAME). This PR replaces them with hardcoded values auth_server_admin / auth_server_admin. Even in demo/test infrastructure, committing hardcoded credentials to git is bad practice and will trigger secret-scanning tools. This should either revert to variables or at minimum be clearly scoped to the demo folder with a strong warning.

  1. Notification endpoint: proof validation silently removed
    oid4vc/oid4vc/public_routes/notification.py

26 lines are deleted that validated JWT proofs in the notification endpoint. The removed comment says "OID4VCI 1.0 §11: if the request body includes a proof…the notification endpoint MUST validate the JWT proof." Removing this is a spec compliance regression — it isn't gated on enable_nonce_endpoint. The PR description doesn't mention this change.

  1. GET /nonce route removed without explanation
    oid4vc/oid4vc/public_routes/registration.py:74-77

The old code registered both GET and POST for /nonce. This PR only registers POST. If any existing wallets use GET /nonce, they'll get a 405. The change has no comment or mention in the PR body.

  1. Logic inversion in handle_proof_of_posession changes existing behavior
    oid4vc/oid4vc/public_routes/token.py:433-473

Old behavior: DB redemption if no c_nonce; direct comparison if c_nonce present.

New behavior: DB redemption if enable_nonce_endpoint=True, ignoring any c_nonce from the token.

The old code branched on whether c_nonce was provided at the call site. The new code always ignores c_nonce when the endpoint is enabled, even if the caller passed one. This is a semantic change that could silently break existing callers that pass c_nonce and expect it to be used.

  1. No test coverage for enable_nonce_endpoint=False path
    oid4vc/oid4vc/tests/routes/test_public_routes.py

The test is updated only for enable_nonce_endpoint=True. The False path (direct c_nonce comparison, no DB hit) has no test. Both the token endpoint's nonce-storage path and handle_proof_of_posession's comparison path should have coverage.

Minor observations

  • Config parsing oid4vc/oid4vc/config.py:53-57: plugin_settings.get("enable_nonce_endpoint") or getenv(...) will treat the string "false" from plugin_settings as truthy before the .lower() in ("true","1","yes") check, since str(enable_nonce_raw).lower() correctly handles it. But an explicit False boolean from plugin_settings (not a string) would be falsy, causing the or to fall through to the env var. A more explicit is not None check would be safer.
  • Config.from_settings called per-request in both token() and handle_proof_of_posession() — fine functionally, just worth noting if profiling ever surfaces this.
  • The .well-known path fix itself is correct per RFC 8615 and an improvement.
  • The README table is a helpful addition for operators.
  • The docker-compose.yaml changes (TENANT_INCLUDE_NONCE: "false" and commenting out STATUS_LIST_FILE_PATH) are reasonable demo config updates.

weiiv added 2 commits June 2, 2026 12:07
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
@weiiv

weiiv commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Hi @swcurran, please see my answer below:

  1. SECURITY — init.sql hardcodes credentials (regression)
    The previous version used psql variable placeholders (:ADMIN_DB_USER, :ADMIN_DB_PASSWORD, :ADMIN_DB_NAME). This PR replaces them with hardcoded values auth_server_admin / auth_server_admin. Even in demo/test infrastructure, committing hardcoded credentials to git is bad practice and will trigger secret-scanning tools.

Fixed. init.sql is now fully parameterized (:ADMIN_DB_USER, :'ADMIN_DB_PASSWORD', :ADMIN_DB_NAME).


  1. Notification endpoint: proof validation silently removed
    26 lines are deleted that validated JWT proofs in the notification endpoint. The removed comment says "OID4VCI 1.0 §11: if the request body includes a proof…the notification endpoint MUST validate the JWT proof." Removing this is a spec compliance regression — it isn't gated on enable_nonce_endpoint.

Intentional. OID4VCI 1.0 §11.1 defines the notification body as notification_id, event, event_description only — there is no proof field to validate.


  1. GET /nonce route removed without explanation
    The old code registered both GET and POST for /nonce. This PR only registers POST. If any existing wallets use GET /nonce, they'll get a 405.

Intentional. OID4VCI 1.0 §7.1 mandates POST /nonce only.


  1. Logic inversion in handle_proof_of_posession changes existing behavior
    Old: DB redemption if no c_nonce; direct comparison if c_nonce present.
    New: DB redemption if enable_nonce_endpoint=True, ignoring any c_nonce from the token.
    The old code branched on whether c_nonce was provided at the call site. The new code always ignores c_nonce when the endpoint is enabled, even if the caller passed one. This could silently break existing callers.

Intentional. When both an external AS issues c_nonce and /nonce is enabled, the wallet puts the /nonce-issued value in its proof (per spec §F.4), but the old "branch on c_nonce presence" compares it against the AS's c_nonce — they never match. Branching on enable_nonce_endpoint is the only correct discriminator. Sole caller is credential.py:277. Docstring added.


  1. No test coverage for enable_nonce_endpoint=False path
    The test is updated only for enable_nonce_endpoint=True. The False path (direct c_nonce comparison, no DB hit) has no test.

Added test_handle_pop_no_nonce_endpoint and test_get_token_nonce_behavior in test_public_routes.py.


Minor — Config parsing or vs is not None
plugin_settings.get("enable_nonce_endpoint") or getenv(...) — an explicit False boolean from plugin_settings would be falsy, causing the or to fall through to the env var. A more explicit is not None check would be safer.

Fixed. Uses is None check; accepts only true/false (boolean or string, case-insensitive).

@jamshale
jamshale merged commit 86c4b0b into openwallet-foundation:main Jun 4, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants